-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pickling for strategies and transformed strategies #1092
Conversation
|
||
|
||
@st.JossAnnTransformer((0.2, 0.2), name_prefix=None) | ||
class JossAnn(axl.Cooperator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seem to be roughly 3 cases.
- a decorated class declaration with no name prefix
- based off of
Player
- based off of another strategy
- based off of
- a decorated class declaration with a name prefix
- see
DoubleFlip
for an example
- see
- a dynamic wrapping of a class (with a name prefix)
- I tried this:
FlipTransformer(name_prefix=None)(Cooperator)()
. It didn't work, and i was going to try to solve it, when i thought, "oh come on! No one would possible try that!"
- I tried this:
i wanted to make sure that each transformer was tested on case-3 and either case-1 or case-2. So these are module level class declarations for case-1, dynamically wrapped strategies for case-3, and some other module level class declarations for special cases and case-2.
and i have a blind-spot for testing overkill (this is already edited down from what i used for testing the changes). problems popped up in strange ways and i got paranoid. pickling problems are weird and have little useful stack trace information when they throw an error. also, i tend to naturally think, "i wonder if you could do this stupid and unintended thing with the library? i think i'll test for that."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok great thanks 👍
@drvinceknight yes. that's correct. Since windows multiprocess requires everything be pickled, that was the aim.
yeah, i've settled for "pickle-able". it does roll off the tongue, though. i've had Peter Piper running through my head for a few days now. |
OK great thanks, that makes sense (and useful to have anyway I guess). Will try to review this first thing tomorrow 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments, questions and requests. Thanks for this. It's looking good as far as I can tell.
|
||
self.assertTrue(is_strategy_static(NewCooperator)) | ||
self.assertFalse(is_strategy_static(NewAlternator)) | ||
|
||
def test_all_strategies(self): | ||
# Attempt to transform each strategy to ensure that implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you were going to add this in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean that i forgot to test IdentityTransformer, right? oops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no!!!! it fails! i think i know why, and it may or may not be possible to fix. yikes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arrrrggghh. brought low by the lowliest of transformers!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops my comment was actually a mistake because I misread the diff but if you've found a fail case that's good I guess although fingers crossed we can get a fix 😕 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phew. fixed. just needed to sleep on it. And it helped me make the code cleaner. :)
axelrod/tests/unit/test_pickling.py
Outdated
def test_pickling_all_strategies(self): | ||
for s in axl.strategies: | ||
player = s() | ||
player.play(axl.Cooperator()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we wrap this in a little loop (and use Alternator
):
opponent = axl.Alternator()
for _ in range(5):
player.play(opponent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about just using assert_mutated_instance_the_same_as_pickled
from line 168?
axelrod/strategy_transformers.py
Outdated
if not player.history: | ||
player.original_player = player.original_class(**player.init_kwargs) | ||
player.use_history = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we call this player.original_player_history
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually @eric-s-s , I'm not sure this is going to work.
The reason we originally didn't just take the history but the player itself is that the Dual must keep track of the state of the original player, so that must include any other internal variables that would get changed by the original player.
I see two course of action:
- Keeping the
original_player
and finding a way to get it to pickle - Add tests with more complex players and check that they both pass with or without this change (no matter what this should probably happen).
- Not only copy the player but copy all the attributes back and forth (least pleasing of all the options in my opinion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theref could you take a look at this and confirm I've got things right? It's not as simple as just using the history, we need to keep track of the whole state, I don't believe the change written here ensures that.
No matter what we should probably write a test that would catch this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the internal state of the original player is what's important, which means all internal variables must also be retained. Another option is that it may be possible to 'rebuild' the original player from the history.
I agree that some tests definitely need to be included that ensure this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that some tests definitely need to be included that ensure this.
Perhaps a test against a player that makes use of a lot of internal variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll comment the code where state-change is recorded, and the possible hidden issue (that was also true with the original)
here's the test i just tried. assert_dual_wrapper_correct
is lifted from code in test_strategy_transformers.py
. (it was a separate method as I used it in a few other tests (which can be mostly be removed)).
def test_all_strategies_with_dual(self):
for s in axl.strategies:
self.assert_dual_wrapper_correct(s)
def assert_dual_wrapper_correct(self, player_class):
p1 = player_class()
p2 = st.DualTransformer()(player_class)()
p3 = axl.CyclerCCD() # Cycles 'CCD'
axl.seed(0)
for _ in range(10):
p1.play(p3)
p3.reset()
axl.seed(0)
for _ in range(10):
p2.play(p3)
self.assertEqual(p1.history, [x.flip() for x in p2.history])
it failed on fails = [axl.EvolvedANN , axl.EvolvedANN5, axl.EvolvedANNNoise05, axl.Golden, axl.e]
after i added a function to switch player.cooperations and player.defections (and then switch them back) they all passed.
Does this work, testing-wise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work, testing-wise?
Yes I think it does. @theref can you confirm?
I expect we would get more failures for more than 10 turns (especially for the complex FSMs) but in the interest of run time I think this is a sufficient test.
after i added a function to switch player.cooperations and player.defections (and then switch them back) they all passed.
The trick will be to get an automatic function that doesn't just switch player.cooperation
and .defections
but every and any attribute that can affect a strategy (take a look at the player equality method, we essentially go in and grab relevant attributes there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested with turns=100 . all tests pass. i'll keep it at turns=100 for the next PR. it adds about 5-10 seconds to the test.
axelrod/strategy_transformers.py
Outdated
def is_strategy_static(player_class): | ||
""" | ||
|
||
:param player_class: Any class that inherits from axelrod.Player |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write a 1 sentence description and you've used sphinx (I believe?) convention here but can you tweak that to be in line with #347.
axelrod/strategy_transformers.py
Outdated
|
||
|
||
class DecoratorReBuilder(object): | ||
def __init__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? I think you can just omit the def __init__
axelrod/strategy_transformers.py
Outdated
|
||
|
||
class StrategyReBuilder(object): | ||
def __init__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment? (I might be wrong and this might be necessary)
return decorator_class(*args, **kwargs) | ||
|
||
|
||
class StrategyReBuilder(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you write a docstring for this.
return isinstance(method, staticmethod) | ||
|
||
|
||
class DecoratorReBuilder(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc string please.
axelrod/strategy_transformers.py
Outdated
if is_static: | ||
# static method | ||
proposed_action = PlayerClass.strategy(opponent) | ||
if strategy_wrapper == dual_wrapper: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've lost me here, isn't this just setting the proposed_action
of anything wrapped with Dual
to be C
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is doing what you said.
but more importantly, it's avoiding calling the wrapped player twice. Dual wrapper requires a proposed_action
variable, but doesn't use it, the C
is just a stand-in value.
The new Dual
changes the state of the actual player when it calls the strategy. Calling the strategy again for proposed_action
would change the state twice for one play.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I still don't see why this is necessary and/or follow. If it's just a minor efficiency boost then lets remove it in the interest of clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(see the explanation of how the state change works first)
wp
= wrapped_player
op
= original_player
the calls inside dual_wrapper
call op.strategy
on wp
. this mutates the state of wp
as if it was op
. if the op.strategy
gets called here again, it will again mutate the state of wp
. this would be bad.
ex of bad way (if uses proposed_action = PlayerClass.strategy(opponent):
player = DualCycler('CD') not yet played
expected_action = C.flip()
= D
cycle now advanced to D
expected_player_state = next_action_to_be_returned: D.flip()
wp.strategy
calledproposed_action
callsop.strategy
- returns
C
(which will be discarded) - mutates state of
wp
(cycle now advanced to 'D') dual_wrapper
calleddual_wrapper
callsop.strategy
- returns
D
- mutates state of
wp
(cycle now advanced toC
, the wrong state) - return action.flip() (which is now
C
, the wrong action)
removing steps 2, 3, 4 makes wp
return the correct action and stay in the correct state.
@@ -15,6 +15,20 @@ class TestClass(object): | |||
|
|||
class TestTransformers(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add specific unit tests for those two extra classes you wrote in strategy_transformers
: DecoratorReBuilder
and StrategyReBuilder
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been done: 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there's a problem with your Dual transformer change.
axelrod/strategy_transformers.py
Outdated
if not player.history: | ||
player.original_player = player.original_class(**player.init_kwargs) | ||
player.use_history = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually @eric-s-s , I'm not sure this is going to work.
The reason we originally didn't just take the history but the player itself is that the Dual must keep track of the state of the original player, so that must include any other internal variables that would get changed by the original player.
I see two course of action:
- Keeping the
original_player
and finding a way to get it to pickle - Add tests with more complex players and check that they both pass with or without this change (no matter what this should probably happen).
- Not only copy the player but copy all the attributes back and forth (least pleasing of all the options in my opinion)
@drvinceknight i'll get to work on all of that in the next few days. just imagine a thumbs-up on every comment |
axelrod/strategy_transformers.py
Outdated
|
||
temp = player.history[:] | ||
player.history = player.use_history[:] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's where the state-change is happening. when the player calls its original_class.strategy
it still affects all the state in the player. so, for instance, a DualFSMPlayer
would still own an FSM, and update its state based on the call to self.FSMPlayer.strategy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but that's the problem, it should have the state of the original player.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my thinking process:
op
= original_player
wp
= wrapped_player
play
only mutates history, cooperations, defections, state_distributions.
all other player attributes are mutated in player.strategy
play_attrs
= history, cooperations, defections, state_distributions
wp
strategy:
- switch to
op.play_attrs
- play
op
strategy - this will update
wp
state as if it wasop
- update history with
op
action - this step could be skipped and done on-the-fly in step 1 (see code below) - switch back to
wp.play_attrs
- flip action
- return (assuming
play
was called, it will now updatewp.play_attrs
. this assumption was also implicit in the original implementation.)
the resulting player is a wp
with wp
history but op
state.
code to switch play_attrs (as yet untested. This is just to get the gist):
def switch_play_attrs(player):
new_history = [action.flip() for action in player.history]
player.history = new_history
temp = player.cooperations
player.cooperations = player.defections
player.defections = temp
new_distributions = defaultdict(int)
for key, val in player.state_distribution.items():
new_key = (key[0].flip(), key[1])
new_distributions[new_key] = val
player.state_distribution = new_distributions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks right.
To ensure I'm following: step 5 and 6 could be swapped? (Not suggesting they are, just checking I'm following).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and step 3 is the important one right? Step 3 is the step that's ensuring the state is updated "properly".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure I'm following: step 5 and 6 could be swapped? (Not suggesting they are, just checking I'm following).
yes, that is correct.
and step 3 is the important one right? Step 3 is the step that's ensuring the state is updated "properly"
yes. step 3 automatically happens as a result of step 2. if it didn't, this would all fall apart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm in the middle of refactoring, but perhaps the new dual_wrapper
code (doc_str edited out) will make things more clear.
def flip_wrapper(player, opponent, proposed_action):
flip_play_attributes(player)
if is_strategy_static(player.original_class):
action = player.original_class.strategy(opponent)
else:
action = player.original_class.strategy(player, opponent)
flip_play_attributes(player)
return action.flip()
flip_play_attributes
flips history
, cooperations
, defections
and (sort of) state_distribution
i've hit a major snag with dual and could use some advice. first, the old version of dual has a bug. The following test passes.
but if any of the
I have a feeling that if i can understand what's going on there, it'll help solve the current problem. current problem: so if you have current "solution" (shudder): i've tried:
I would love some ideas. and/or i would love to know what's going on in the original. |
Am I correct in understanding that this bug "only" affects using the Dual twice? |
yes, that is correct. |
I would be quite tempted to simply include a catch if that's ever called and raise an error. From the use case point of view being able to call them twice is not really necessary (these Duals are mainly used in fingerprinting). Obviously ideally and for completeness it would be nice to have but if this makes the code too ugly/hard to maintain and/or is not possible I'd be happy to just raise an error saying "Calling the Dual recursively is not implemented." |
hmmmmm. i've found a solution that works beautifully, but is very programmatically questionable (it passes a set of ugly test)
mixed with
so, the player_class is responsible for all the work of the dual_wrapper. in this case, it passes all the tests that original dual_wrapper failed on and the tests that the new dual_wrapper failed on. |
it adds a specialized code for |
Ok I my initial thought it that this is a nice/ok solution. We should document in the dual wrapper why it has been implemented differently. |
i'll push that up soon and we can see how it looks |
I'll review properly tomorrow 👍 Thanks :) |
axelrod/strategy_transformers.py
Outdated
proposed_action = PlayerClass.strategy(opponent) | ||
else: | ||
proposed_action = PlayerClass.strategy(self, opponent) | ||
|
||
if strategy_wrapper == dual_wrapper: | ||
# after dual_wrapper_figures calls the strategy, it returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo here - "dual_wrapper_figures" should be "dual_wrapper". i'll wait on a full review before pushing this change.
axelrod/tests/unit/test_pickling.py
Outdated
klass = st.FlipTransformer()(axl.Cooperator) | ||
klass = st.DualTransformer()(klass) | ||
player = st.FinalTransformer((C, D))(klass)() | ||
copy = pickle.loads(pickle.dumps(player)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be self.assert_original_equals_pickled(player)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good.
If you could address the minor things I've left and I'd appreciate it if @theref (original author) could take a look at the way you've done the Dual
just to make sure I'm not missing anything but you've written a great set of tests so I'm pretty sure we're fine.
I'm correct that test_dual_transformer_special_cases_1
and test_dual_transformer_special_cases_2
are the test cases that need to the Dual re write yes?
axelrod/strategy_transformers.py
Outdated
return self_.__class__, (), self_.__dict__ | ||
|
||
decorators = [] | ||
for klass in self_.__class__.mro(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class_
perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or child_class
or sub_class
might be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with that also.
@@ -141,6 +153,27 @@ def __repr__(self): | |||
prefix = ', ' | |||
return name | |||
|
|||
def reduce_for_decorated_class(self_): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while checking PEP8 about klass
, i noticed about the importance of sticking with self
and cls
. even though it shadows a variable from outer scope, is it better to shadow the self
variable than to use self_
?
here's the quote
Function and method arguments
Always use self for the first argument to instance methods.
Always use cls for the first argument to class methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not terribly sure to be honest. I'm happy as is, I'm guessing that quote does not necessarily apply to nested def and class?
axelrod/strategy_transformers.py
Outdated
class_module = import_module(self_.__module__) | ||
import_name = self_.__class__.__name__ | ||
|
||
if player_can_be_pickled(self_): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this can we use a try/except
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure how to do it here. no error is raised in the __reduce__
function, so it wouldn't be able to catch it, right? since this is called by the pickler, that is where you'd need the try/catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds like you've already thought about it. 👍
axelrod/tests/unit/test_pickling.py
Outdated
self.assert_orignal_equals_pickled(player) | ||
|
||
def test_created_on_the_spot_multiple_transformers(self): | ||
klass = st.FlipTransformer()(axl.Cooperator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class_
? (not terribly insistent on this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i picked "klass" as it seems to be convention in a lot of code. i also have no real preference.
then i searched on PEP8 and here's what i found.
If a function argument's name clashes with a reserved keyword, it is generally better to append a single trailing underscore rather than use an abbreviation or spelling corruption. Thus class_ is better than clss . (Perhaps better is to avoid such clashes by using a synonym.)
so class_
(or something more verbose) is the official answer. Learn something new everyday!
axelrod/tests/unit/test_pickling.py
Outdated
|
||
def assert_original_plays_same_as_pickled(self, player, turns=10): | ||
copy = pickle.loads(pickle.dumps(player)) | ||
opponent_1 = axl.CyclerCCCDCD() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we loop this over a collection of opponents:
for opponent_class in [axl.Defector(), axl.Cooperator(), axl.Random(), axl.CyclerCCCDC]:
axelrod/tests/unit/test_pickling.py
Outdated
def assert_mutated_instance_same_as_pickled(self, player): | ||
player.reset() | ||
turns = 5 | ||
opponent = axl.Alternator() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about looping over a set of opponents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on second look, these two tests can be combined and save some time (which will be taken up and more with extra strategies). i'll just run
def assert_equals_instance_from_pickling(original_instance):
copy = pickle.loads(pickle.dumps(original_instance))
self.assertEqual(copy, original_instance)
after every Match. i'll have a nice batch of already mutated instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
axelrod/tests/unit/test_pickling.py
Outdated
self.assert_mutated_instance_same_as_pickled(player) | ||
|
||
def test_dual_transformer_special_case(self): | ||
player = TestDualTransformerIssues() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't terribly clear? (even the class definition before)
Could the class def be moved here and made a bit more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, this can't be moved inside the test. no pickling of non-module-lvl classes. i'll work on clarity in class name, test name, and perhaps comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool.
|
||
self.assertEqual(p1.history, [x.flip() for x in p2.history]) | ||
def test_dual_transformer_special_cases_1(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right that these tests are the ones that are fixed by your re write of Dual?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that is correct. it addresses issues with the original code and a few different tries on the re-write. i'll try to make this more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left one small stylistic comment for a crazy set of nested function calls but that's not a deal breaker for me.
I think this is basically good to go now. I would still like @theref to take a look and approve the change to DualTransformer
.
axelrod/tests/unit/test_pickling.py
Outdated
self.assert_orignal_equals_pickled(player) | ||
|
||
interspersed_dual_transformers = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't terribly nice to look at. Perhaps we can make use of intermediate variables to improve readability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wasn't sure how to go about it. i'll do that.
@@ -15,6 +15,20 @@ class TestClass(object): | |||
|
|||
class TestTransformers(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been done: 👍
multiple_dual_transformers = DualTransformer()(DualTransformer()(axelrod.WinStayLoseShift)) | ||
self.assert_dual_wrapper_correct(multiple_dual_transformers) | ||
|
||
def assert_dual_wrapper_correct(self, player_class): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I really like this test, great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @theref 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd love to take credit, but my contribution to this particular test was pulling it out of the original testing code to make a function and adding axl.seed(0)
. whoever wrote the original (test_dual_somestrategy
) made a nice concise test that showed me exactly what dual_transformer
needed to do. if that was you, @theref , thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me -- just a couple of typos / docstring requests. Thanks for taking this on!
axelrod/strategy_transformers.py
Outdated
@@ -9,9 +9,12 @@ | |||
import copy | |||
import inspect | |||
import random | |||
from typing import Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reorder the imports (alphabetical, std lib, then 3rd party, then project specific)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to know!
@@ -57,6 +60,14 @@ def __init__(self, *args, **kwargs): | |||
else: | |||
self.name_prefix = name_prefix | |||
|
|||
def __reduce__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a docstring here? I get that we're rebuilding the class but if I didn't know what was going on I wouldn't know until I read through the file, and this is a fairly complicated part of the library so I'd like to err on the side of more documentation.
axelrod/strategy_transformers.py
Outdated
proposed_action = PlayerClass.strategy(opponent) | ||
else: | ||
proposed_action = PlayerClass.strategy(self, opponent) | ||
|
||
if strategy_wrapper == dual_wrapper: | ||
# after dual_wrapper calls the strategy, it returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capital After
axelrod/strategy_transformers.py
Outdated
Flips all the attributes created by `player.play`: | ||
- `player.history`, | ||
- `player.cooperations`, | ||
- `player.defectsions`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defections
@eric-s-s Please rebase onto master. |
did the rebase work correctly? it's the first time i tried it and i suspect it did not.
as far as i can tell, all my changes are here. i have no idea what is going on with all the other changed files, though. should i not have pulled from upstream before doing this? |
I'm not sure what you actually did -- it looks like there are twice as many commits. When I tried to rebase again I got a ton of merge conflicts, so I suspect that you somehow rewrote the commits then pushed them to the head of the old branch, while picking up some old commits. Maybe you rebased master onto your branch? I was able to squash all the commits, see #1106. I think that #1107 has the original commits, rebased. |
for future reference, did steps 1-4 follow correct procedure? on the git-rebase man-page it looked so simple and straightforward :( . i reset the head and tried it again. i had the same merge conflicts and resolved in the same way. looking at the git log, it looks fine. perhaps it was step 6? when i went to push up the rebased branch, i had a conflict and git suggested that i git pull. should i have pushed up the rebased branch as a new PR? |
Yeah I think that was the error, you should have |
@drvinceknight thanks. i reset the head, re-rebased it and am going to try that now. looking at the insghts/graphs/network and the files_changed, it seems to have worked. if that's the case, maybe merge this one and close the other two? |
I think this is fine to merge now and we can close the others. |
I'm just going to re start the builds to triple check nothing got lost during the rebases. |
Thanks @eric-s-s 👍 Great work getting to the bottom of this. 👍 |
it was my pleasure. :) |
#718
had to change
test_fingerprint.py
because theaxl.seed
behavior has changed forDualTransformer
.DualTransformer
still passes all its own tests.The equality testing to Game was added so that I could test
pickle.loads(pickle.dumps(player)) == player
. Since it creates a new Game instance, these tests evaluated to false until i added the__eq__
method toGame
.